Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Initial scaffolding for operator #4117

Conversation

wfernandes
Copy link
Contributor

@wfernandes wfernandes commented Jan 25, 2021

What this PR does / why we need it:
This PR adds the initial bare bones scaffolding for the CAPI operator.

kubebuilder init --domain operator.cluster.x-k8s.io
# Created Resource and Controller
kubebuilder create api --group operator --version v1alpha4 --kind CoreProvider
# Created Resources and no Controllers for each of the following 
# since we are going to have a single controller reconciler for all the resources.
kubebuilder create api --group operator --version v1alpha4 --kind BootstrapProvider
kubebuilder create api --group operator --version v1alpha4 --kind ControlPlaneProvider
kubebuilder create api --group operator --version v1alpha4 --kind InfrastructureProvider

Other Notes

  • The API will be incrementally added in.
  • make commands coming in a separate PR.
  • Dockerfile updates in a separate PR.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Ref #3833
Fixes #4118

/area api
/area operator

@k8s-ci-robot
Copy link
Contributor

@wfernandes: The label(s) area/operator cannot be applied, because the repository doesn't have them

In response to this:

What this PR does / why we need it:
This PR adds the initial bare bones scaffolding for the CAPI operator.

kubebuilder init --domain operator.cluster.x-k8s.io
# Created Resource and Controller
kubebuilder create api --group operator --version v1alpha4 --kind CoreProvider
# Created Resources and no Controllers for each of the following 
# since we are going to have a single controller reconciler for all the resources.
kubebuilder create api --group operator --version v1alpha4 --kind BootstrapProvider
kubebuilder create api --group operator --version v1alpha4 --kind ControlPlaneProvider
kubebuilder create api --group operator --version v1alpha4 --kind InfrastructureProvider

Other Notes

  • The API will be incrementally added in.
  • make commands coming in a separate PR.
  • Dockerfile updates in a separate PR.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Ref #3833

/area api
/area operator

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added area/api Issues or PRs related to the APIs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 25, 2021
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 25, 2021
@wfernandes
Copy link
Contributor Author

/cc @fabriziopandini @CecileRobertMichon

I know this PR looks enormous but it's just a few kubebuilder commands that generates some initial scaffolding.

I'm trying to break out the work for the operator into smaller PRs with respective issues so that the work isn't too overwhelming.

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wfernandes thanks!
I have added a couple of comments, mostly about changes required in order to make the default Kubebuilder scaffolding more consistent/integrated with the rest of the codebase.
Up to you if to tackle this comments within this PR or in follow up PRs.

operator/.gitignore Outdated Show resolved Hide resolved
operator/Dockerfile Outdated Show resolved Hide resolved
operator/Dockerfile Outdated Show resolved Hide resolved
operator/Dockerfile Outdated Show resolved Hide resolved
operator/Makefile Outdated Show resolved Hide resolved
operator/go.mod Outdated Show resolved Hide resolved
operator/hack/boilerplate.go.txt Outdated Show resolved Hide resolved
operator/main.go Outdated Show resolved Hide resolved
operator/main.go Outdated Show resolved Hide resolved
operator/main.go Outdated Show resolved Hide resolved
@fabriziopandini
Copy link
Member

fabriziopandini commented Jan 28, 2021

Changes LGTM to me given the assumption this is plain "kubebuilder" scaffholding and #4126 is going to make everything more consistent/integrated with the rest of the codebase (thus addressing above comments).

@wfernandes wfernandes force-pushed the capi-operator-scaffolding branch from 9404d34 to 94f2983 Compare January 29, 2021 16:28
@wfernandes
Copy link
Contributor Author

@fabriziopandini @CecileRobertMichon LMK if y'all think other changes are needed in this specific PR.

Just FYI, I have #4126 and #4132 in the queue ready to be rebased and reviewed as well. So there may be suggestions or changes that have already been addressed.

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2021
@vincepri
Copy link
Member

vincepri commented Feb 1, 2021

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2021
@wfernandes wfernandes marked this pull request as draft February 9, 2021 22:50
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 9, 2021
@wfernandes wfernandes changed the base branch from master to operator-0.4 February 23, 2021 18:20
@alexander-demicev alexander-demicev force-pushed the capi-operator-scaffolding branch from 94f2983 to eae016e Compare February 25, 2021 16:51
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2021
@alexander-demicev
Copy link
Contributor

I moved the code to exp directory and did a rebase from main. @fabriziopandini @CecileRobertMichon

@@ -16,15 +16,16 @@ at the bottom of this page for full details of MachineHealthCheck limitations.

## What is a MachineHealthCheck?

A MachineHealthCheck is a resource within the Cluster API which allows users to define conditions under which Machines within a Cluster should be considered unhealthy.
A MachineHealthCheck is a resource within the Cluster API which allows users to define conditions under which Machines within a Cluster should be considered unhealthy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those changes seems from a commit not relevant for this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, since the operator is in a separate branch, these commits come from a rebase. I can remove them from the PR.

Copy link
Contributor

@alexander-demicev alexander-demicev Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary commits removed, all should be fine now.

@alexander-demicev alexander-demicev force-pushed the capi-operator-scaffolding branch from eae016e to 606c913 Compare March 1, 2021 11:33
@fabriziopandini
Copy link
Member

As per #4117 (review) this PR introduces kubebuilder standard scaffolding, but there are changes to be applied in order to make things more consistent with the CAPI repository.

I'm ok to get these point fixed in follow up PRs, but given that now more people are going to work on this I would like to get them tracked somewhere (one or more new issues, or a comment in existing ones).

For your convenience below the list of things captured in the previous review:

  • avoid a new go.mod file
  • avoid a new .gitignore and rely on the top level one (e.g. we are already doing this for KCP, CABPK)
  • avoid a new docker file and rely on the top level one (e.g. we are already doing this for KCP, CABPK)
  • avoid a new boilerplate.txt file and use the one under hack/
  • the make file should be made consistent with other makefiles in CAPI
    • drop targets like run and provide a consistent dev UX (tilt)
    • drop fmt/vet in favour of golangci for consistency with the rest of the codebase
    • use the controller-gen under hack/tools/
    • use a consistent build/package (publish) strategy
  • use a unique name for the certificate secret
  • target a new namespace (capi-operator-system)
  • prefix all the object names with capi-operator
  • add the clusterctl.cluster.x-k8s.io/core="operator" label to all the objects
  • introduce manager_image_patch.yaml manager_pull_policy.yaml patches (see consistent dev UX)
  • add the node-role.kubernetes.io/master toleration
  • make roles consistent with other operators (1 file with all the roles)
  • drop samples
  • use cert-manager v1 API (new)
  • remove Log field from the reconciler (and use the one from the context)
  • use common names for the flags (e.g. metrics-bind-addr instead of metrics-addr)
  • switch to klog for consistency with the rest of the codebase
  • switch from ginkgo to go test

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
  • Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 1, 2021
@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 1, 2021
@alexander-demicev
Copy link
Contributor

@fabriziopandini Thank you for your review and suggestions. Some of your points already might've been addressed in 2 other PRs related to the operator. I do agree this should all be tracked and fixed in follow-up PRs.

@wfernandes wfernandes force-pushed the capi-operator-scaffolding branch from 606c913 to ec84695 Compare March 3, 2021 02:57
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 3, 2021
Domain is cluster.x-k8s.io

Create API scaffolding for CoreProvider

kubebuilder create api --group operator --version v1alpha4 --kind CoreProvider

Create API scaffolding for BootstrapProvider

kubebuilder create api --group operator --version v1alpha4 --kind BootstrapProvider
Created Resources, not controllers

Create API scaffolding for ControlPlaneProvider

kubebuilder create api --group operator --version v1alpha4 --kind ControlPlaneProvider
Created Resources, not controllers

Create API scaffolding for InfrastructureProvider

kubebuilder create api --group operator --version v1alpha4 --kind InfrastructureProvider
Created Resources, not controllers

Add placeholders for ProviderSpec and ProviderStatus

Fix some module imports. Will be cleaned up in future PRs.

Fix boilerplate on files

Move operator to exp folder
@alexander-demicev alexander-demicev force-pushed the capi-operator-scaffolding branch from ec84695 to ced6551 Compare March 3, 2021 11:10
@alexander-demicev
Copy link
Contributor

@fabriziopandini CLA check now passes and commits are squashed.

@fabriziopandini
Copy link
Member

given that future work is tracked in #4246
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2021
@CecileRobertMichon
Copy link
Contributor

@alexander-demichev this is still a draft PR, are you ready to move it to PR and remove the hold?

@alexander-demicev
Copy link
Contributor

it's not a draft PR anymore, should be ready for merging.

@CecileRobertMichon
Copy link
Contributor

it's not a draft PR anymore, should be ready for merging.

I still see "draft" at the top under the title, and it still has WIP and hold labels. I don't see an option to publish the PR, maybe only @wfernandes can do it since he's the PR author. If not you might have to open a new PR from the same branch so we can merge it @alexander-demichev.

@wfernandes wfernandes marked this pull request as ready for review March 4, 2021 01:46
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2021
@fabriziopandini
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2021
@alexander-demicev
Copy link
Contributor

@CecileRobertMichon the PR should now be ready for merging.

@CecileRobertMichon
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2021
@k8s-ci-robot k8s-ci-robot merged commit 48f7531 into kubernetes-sigs:operator-0.4 Mar 4, 2021
@alexander-demicev alexander-demicev deleted the capi-operator-scaffolding branch March 4, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Issues or PRs related to the APIs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate initial scaffolding for the operator
7 participants